Improve CMake correctness and handling #1143
Conversation
The more general CMake way to handle library suffixing is to set CMAKE_<CONFIG>_POSTFIX, so setting the Debug output suffix name should be more correctly done by the caller or CMake configurer by setting the desired value in CMAKE_DEBUG_POSTFIX.
As of CMake 3.0 with CMP0042, MACOSX_RPATH is enabled by default. Since the validated version used by jsoncpp is later than 3.0, this is already covered.
Since CMake has subdirectory variable scope, unilaterally set the CMAKE_CXX_STANDARD variable to use C++11. This covers cases with the library being included externally, both in cases of only C++98 being specified, as well as later versions being specified (since the CXX_STANDARD itself isn't a library dependency, only the PUBLIC target_compile_features on jsoncpp_lib). The previous direct check for C++98 is handled by requiring C++11 on this library; should the compiler being used not support C++11 then CMake will issue an error.
Since the introduction of CMAKE_COMPILER_IS_GNUCXX CMake has suggested using CMAKE_CXX_COMPILER_ID for more general checks.
EXTRA_CXX_FLAGS is never defined, making this a noop. Further, COMPILE_OPTIONS is invalid to set as a DIRECTORY property.
Commit aebc7fa added version checks for CMake compatibility. In reality, only the add_compile_definitions need the check - add_compile_options itself has been supported since 3.0. Tested and confirmed built successfully with CMake 3.8.0.
This is already covered by the toplevel CMake, which also serves to provide a consistent minimum version.
|
LGTM. |
|
@hjmjohnson Hi Hans, Would you like to take a look at this? |
|
@mrjoel, I like this a lot. Thank you! We rely on users to keep the cmake files up-to-date. I am tempted to merge this immediately, but I would like to retain all your commits, for future reference. That means we need to rebase and create a merge-commit, the one area where BitBucket is better than GitHub. Would you please rebase your branch, and then create a merge-commit? You will need to use drop-down, since if you simply click "rebase and merge" you would be putting all these commits directly on the |
|
Sure rebasing is fine, but I'm not entirely clear what you're asking for in regards to the merge commit using drop-down (I mainly use git from commandline). You want a single commit for merging which reflects a merge of this branch to master? |
|
@cdunn2001 Hmm, inadvertently closed when pushing the rebase branch, and I don't seem to have permissions to reopen. Will a maintainer with perms reopen, or should I create a new PR? |
|
The branch |
|
The branch still exists, just not referenced anymore... perhaps I did a delete with new push instead of force push to update the branch... |
|
Oh, I think I could have re-opened it if you'd simply recreated the branch, but now I can't because you already created a new PR for the same branch. Not sure, but I guess that's how GitHub works. Not a big deal though. |
Update the CMake processing to improve interoperability with being included as CMake from a larger project using jsoncpp.